Skip to content

Should use copy of remotes.listenerTree? - #1966

Closed
timneedham wants to merge 1 commit into
strongloop:masterfrom
timneedham:patch-1
Closed

Should use copy of remotes.listenerTree?#1966
timneedham wants to merge 1 commit into
strongloop:masterfrom
timneedham:patch-1

Conversation

@timneedham

Copy link
Copy Markdown
Contributor

Hi,

This is my first ever pull request (apologies if it sucks)...

As part of investigating Loopback issue #1951 with @gunjpan's assitance (whereby creating models/relations programatically appears to cause model.nestRemoting() to throw a TypeError in the absence of a remotes listenerTree) I think I found something here:

https://github.com/strongloop/loopback/blob/master/lib/model.js#L805-L810

  • From what I can tell, a local copy has been made of remotes.listenerTree, which has been defaulted to handle the absence of a listenerTree, but shortly-after the original un-defaulted value is used, and not the local copy. That's when I hit the TypeError.

A quick change to use the local copy (as per the PR) fixes the immediate issues I was facing in #1951.

Hope this helps?

Tim

@slnode

slnode commented Jan 17, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@bajtos

bajtos commented Feb 4, 2016

Copy link
Copy Markdown
Member

@slnode ok to test

@slnode

slnode commented Feb 4, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@bajtos bajtos self-assigned this Feb 4, 2016
@bajtos

bajtos commented Feb 4, 2016

Copy link
Copy Markdown
Member

Hi @timneedham, thank you for the pull request. Could you please add a unit-test to reproduce the problem and verify that your change fixed it?

@timneedham

Copy link
Copy Markdown
Contributor Author

Hi @bajtos,

I think I've hit my current ceiling at the moment... I did a repo to illustrate the problem here:

https://github.com/timneedham/nest-remoting-issue

...which @gunjpan confirmed in #1951 : apologies, I have tried, expressing this as a unit-test is a bit out of my grasp at the moment.

Kind regards

Tim

@slnode

slnode commented Feb 24, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@bajtos

bajtos commented Apr 19, 2016

Copy link
Copy Markdown
Member

@timneedham apparently this bug is not possible to reproduce in our Mocha-based test suite :( I opened a new PR to make sure nothing breaks in the current code base, and also reworded the commit message - see #2245.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants